[GraphQL][DotMove][1/n] Introduces DotMove resolution
#18774
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
manolisliolios
changed the title
DotMove resolutionDotMove resolution
manolisliolios
force-pushed
the
ml/dot-move-1-n
branch
from
ba4d77d to
02f885f
Compare
This was referenced Jul 25, 2024
manolisliolios
requested review from
amnn,
wlmyng,
stefan-mysten,
emmazzz and
suiwombat
as code owners
amnn
reviewed
Jul 26, 2024
There was a problem hiding this comment.
Very nice overall! Here's the high level themes:
- Can we publish the real
dotmovepackage instead of a test version? - When outputting logs from tests, use
eprintln!instead ofprintln!-- I believe test runners treat stderr specially and will show its output when a test fails. - Naming -- it's probably wise to us a generic name here, in case the name changes before launch and the code has already gone out with a release of GraphQL -- changing the name after the fact would be a pain, so it's better to use a generic name and keep it that way even when a new name is chosen.
- Use
pub(crate)whenever you can -- that way the language server can help identify unused functions. - The new
configmodule is handling a lot of non-config things, PTAL at the suggestion reorganisation there.
crates/sui-graphql-rpc/tests/dot_move/dot_move/sources/dotmove.move
Outdated
Show resolved
Hide resolved
|
Thanks a lot for the thorough review @amnn !
I initially considered that, but... I think it helps a lot not having to deal with so many more objects, read paths (e.g. direct DFs additions versus table access), access control for different calls (from capabilities), etc. As the on-chain package evolves, the types would remain the same and we only care about deserializing those properly.
Agreed. Naming... oof... that's harder than building this.
Agreed! I kinda felt I needed to do it, not sure why I hesitated! And thanks for the other points! |
Got it -- I'll leave it up to you, as I think this is mostly a maintenance question for you! |
manolisliolios
force-pushed
the
ml/dot-move-1-n
branch
from
3536f66 to
5d951a7
Compare
amnn
approved these changes
Jul 31, 2024
There was a problem hiding this comment.
Looks good! Only remaining comment is to rehome the MoveRegistryConfig within the main config.rs, but this is looking great -- thanks @manolisliolios !
crates/sui-graphql-rpc/src/types/dot_move/named_move_package.rs
Outdated
Show resolved
Hide resolved
manolisliolios
force-pushed
the
ml/dot-move-1-n
branch
from
bc35f47 to
c57edac
Compare
|
As my stack has landed, this PR can now rebase and take advantage of |
manolisliolios
force-pushed
the
ml/watermark-new-task
branch
2 times, most recently
from
d48be10 to
1dd99f9
Compare
manolisliolios
force-pushed
the
ml/dot-move-1-n
branch
from
c57edac to
b845f77
Compare
manolisliolios
force-pushed
the
ml/dot-move-1-n
branch
from
a3a570c to
3331aee
Compare
manolisliolios
force-pushed
the
ml/dot-move-1-n
branch
from
3331aee to
7e3e6f2
Compare
## Description Introduces `typeByName` that resolves a given type using the equivalent dot move name. ## Test plan You can run `cargo test` for unit tests (parsing of types). For e2e tests: ``` cargo nextest run --package sui-graphql-rpc --test dot_move_e2e --features pg_integration ``` ## Stack - #18774 - #18770 --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
## Description Introduces the external resolution, which requests "names" from a different graphql node. In reality, that'll be a mainnet RPC endpoint being used. ## Test plan e2e tests updated to start a secondary service that utilizes external resolution, and depend on that: ``` cargo nextest run --package sui-graphql-rpc --test dot_move_e2e --features pg_integration ``` ## Stack - #18797 - #18774 - #18770 --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
jangid
pushed a commit
to jangid/sui
that referenced
this pull request
Aug 30, 2024
## Description Introduces the logic for querying packages & types by name. Works for both internal & external resolution mode. Important: There's not yet an officially supported package for this functionality on mainnet. There'll be a follow-up PR to address this. ## Test plan There are both unit tests on name parsing, and e2e tests for package resolution. I test package upgrades, and resolution both with "latest", as well as on given fixed versions. ``` cargo nextest run --package sui-graphql-rpc --test dot_move_e2e --features pg_integration ``` ## Stack - MystenLabs#18770 --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [x] GraphQL: Introduces `.move` name resolution (internal & external) for GraphQL. Only supported on a non-mainnet environment for the time being. - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
suiwombat
pushed a commit
that referenced
this pull request
Sep 16, 2024
## Description Introduces the logic for querying packages & types by name. Works for both internal & external resolution mode. Important: There's not yet an officially supported package for this functionality on mainnet. There'll be a follow-up PR to address this. ## Test plan There are both unit tests on name parsing, and e2e tests for package resolution. I test package upgrades, and resolution both with "latest", as well as on given fixed versions. ``` cargo nextest run --package sui-graphql-rpc --test dot_move_e2e --features pg_integration ``` ## Stack - #18770 --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [x] GraphQL: Introduces `.move` name resolution (internal & external) for GraphQL. Only supported on a non-mainnet environment for the time being. - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is the first PR of the stack that introduces the
internal.move resolution, and the e2e testing for it.There are some configs that will make sense in the follow-up PRs, didn't want to over-do it with the splitting! 😅
On the testing side, I split out the
networkrelated stuff with thegraphqlstuff on theCluster, so I can start-up a network (validator, fn, indexer) easily, and decouple the init of the GQL Service.That'll be handy on my follow-up PRs.
Test plan
There are both unit tests on name parsing, and e2e tests for package resolution.
I test package upgrades, and resolution both with "latest", as well as on given fixed versions.
Stack
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.
.movename resolution (internal and external) for GraphQL. Supported only on a non-Mainnet environment for now.